Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Diagnostics workers - fix GTL and toolbar #3181

Merged
merged 12 commits into from
Jan 11, 2018
Merged

Diagnostics workers - fix GTL and toolbar #3181

merged 12 commits into from
Jan 11, 2018

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Jan 5, 2018

Ops > Diagnostics, Server detail, Workers tab

In fine, the screen was a list where clicking an item would simply select the item (unselecting any other), without changing to a detail screen.

Now, we have a checkbox with each item, but clicking the item causes an exception (and the toolbar button to restart the worker doesn't work).


This PR:

  • changes the toolbar button to use the standard onwhen logic for disabled
  • makes the toolbar button work
  • adds a new GTL option - :clickable, setting to false causes clicking on items to do nothing
  • updates ReportDataController(js) to handle some items not having links.
  • cleans up the dead code the old list used

https://bugzilla.redhat.com/show_bug.cgi?id=1531524

@himdel
Copy link
Contributor Author

himdel commented Jan 6, 2018

The screen in fine:
workers58

in gaprindashvili:
workers59

@himdel himdel changed the title [WIP] Diagnostics workers - fix GTL [WIP] Diagnostics workers - fix GTL and toolbar Jan 6, 2018
@himdel
Copy link
Contributor Author

himdel commented Jan 6, 2018

wip - loose ends in c9674ee, seems like there's some method to do something on this screen in js, just used in a way that doesn't work?

himdel added 11 commits January 9, 2018 12:46
with the new gtl, we don't need to reload the whole screen on selection

thus, the "can only restart 1 worker at a time" logic must move to JS (at least until we start supporting restarting multiple workers)
…orker_id], drop that

this removes `@sb[:selected_worker_id]` completely, in favor of using `checked_or_params` to get the selected worker
the method was called by the old gtl (`gtl/_list`) on row selection,
new GTL uses the standard checked_or_params way
before, `pm_get_workers` would reset `@sb[:selected_worker_id]` and call `get_workers`

nothing else calls `get_workers` directly, and the id is no longer used

merging into 1 method :)
looks like defaulting to current controller on nil, etc. should happen there, not in JS
false means clicking the entry will do nothing
this is achieved by setting showUrl to false,

ReportDataController#onItemClicked already does nothing when showUrl is false
when `initObject.showUrl` is "", the defaults from initComponent kick in

but the logic in onItemClicked ignores clicks when showUrl is falsy

so.. logically, false is the right value

except there's already code which assumes showUrl is always a string - adding explicit checks
@himdel himdel changed the title [WIP] Diagnostics workers - fix GTL and toolbar Diagnostics workers - fix GTL and toolbar Jan 9, 2018
@himdel himdel removed the wip label Jan 9, 2018
@miq-bot
Copy link
Member

miq-bot commented Jan 10, 2018

Checked commits https://github.com/himdel/manageiq-ui-classic/compare/e16c2c0051222d3a7e896aeff795264da5868661~...fc5fd8234e920c57021d7719878201339cff6739 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 1 offense detected

app/helpers/application_helper/toolbar/diagnostics_server_center.rb

@himdel himdel closed this Jan 10, 2018
@himdel himdel reopened this Jan 10, 2018
@himdel
Copy link
Contributor Author

himdel commented Jan 11, 2018

Cc @dclarizio

@mzazrivec mzazrivec self-assigned this Jan 11, 2018
@mzazrivec mzazrivec added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 11, 2018
@mzazrivec mzazrivec merged commit 6ac8672 into ManageIQ:master Jan 11, 2018
@himdel himdel deleted the workers-gtl-bz1531524 branch January 11, 2018 13:06
simaishi pushed a commit that referenced this pull request Jan 11, 2018
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit e4e1ee3c3c399ca559401069af6cfa3085a8496c
Author: Milan Zázrivec <[email protected]>
Date:   Thu Jan 11 14:05:45 2018 +0100

    Merge pull request #3181 from himdel/workers-gtl-bz1531524
    
    Diagnostics workers - fix GTL and toolbar
    (cherry picked from commit 6ac867275ceffc6a9c18405058509ec2a480302d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1533548

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants